-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integration test fix #112
Integration test fix #112
Conversation
ce2cdce
to
c719ecd
Compare
c719ecd
to
d557030
Compare
underlying cause was a background thread which continued to send commands after a disconnect. Additionally logging was improved
d557030
to
7af162f
Compare
Also improve logging of exception
6b5d3e3
to
782088b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments
@@ -61,7 +68,7 @@ public AbstractIntegrationTest() { | |||
try { | |||
final URL dockerComposeURL = AbstractIntegrationTest.class.getClassLoader().getResource("docker-compose.yml"); | |||
final Path dockerComposeFile = Paths.get(dockerComposeURL.toURI()); | |||
final BackgroundExecutor backgroundExecutor = Client.getService(BackgroundExecutor.class); | |||
final Executor backgroundExecutor = Executors.newCachedThreadPool(new SimpleThreadFactory()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know if change is correct. We should discuss this next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only in a test and the executor is only used to startup the docker containers
}); | ||
useLongPolling.set(longPoll); | ||
connectedFlag.set(true); | ||
uiExecutor.execute(() -> connectionFlagForUiExecutor = true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this all be done in parallel? Maybe the tasks that are submitted to the uiExecutor and the backgroundExecutor will executed long after the lock is released
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the change of the flag executed in the uiExecutor in the first place?
switching the flag should be possible from any thread
@@ -105,6 +105,7 @@ public String getHeaderField(String name) { | |||
|
|||
final ClientModelStore clientModelStore = new ClientModelStore(new DefaultModelSynchronizer(() -> null)); | |||
final HttpClientConnector connector = new HttpClientConnector(getDummyURL(), uiExecutor, backgroundExecutor, clientModelStore, new JsonCodec(), new SimpleExceptionHandler(), Client.getService(HttpClient.class)); | |||
connector.connect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this end in problems since we only have a dummy endpoint? I would assume that an exception will be thrown in a background thread.
No description provided.